Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

checkpolicy/oss-fuzz: add libfuzz based fuzzer #313

Closed
wants to merge 15 commits into from

Conversation

cgzones
Copy link
Contributor

@cgzones cgzones commented Sep 30, 2021

Introduce a libfuzz1 based fuzzer testing the parsing and policy
generation code used within checkpolicy(8) and checkmodule(8), similar
to the fuzzer for secilc(8).
The fuzzer will work on generated source policy input and try to parse,
link, expand, optimize, sort and output it.

Build the fuzzer in the oss-fuzz script.

/cc @fishilico @evverx

I am not familiar how the actual integration into oss-fuzz works and if it needs any update.

@evverx
Copy link
Contributor

evverx commented Sep 30, 2021

@cgzones thanks a lot for working on this! I'll take a look tomorrow.

I'm not sure why CIFuzz didn't report a memory leak it found though:

2021-09-30T16:25:19.7597616Z =================================================================
2021-09-30T16:25:19.7598142Z ==21==ERROR: LeakSanitizer: detected memory leaks
2021-09-30T16:25:19.7598585Z 
2021-09-30T16:25:19.7599025Z Direct leak of 48 byte(s) in 1 object(s) allocated from:
2021-09-30T16:25:19.7600239Z     #0 0x52515d in __interceptor_malloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:129:3
2021-09-30T16:25:19.7601187Z     #1 0x57d50f in define_cexpr /src/selinux/checkpolicy/policy_define.c:3751:14
2021-09-30T16:25:19.7601985Z     #2 0x587f2c in yyparse /src/selinux/checkpolicy/policy_parse.y:573:44
2021-09-30T16:25:19.7603088Z     #3 0x55e2c7 in read_source_policy /src/selinux/checkpolicy/fuzz/checkpolicy-fuzzer.c:81:6
2021-09-30T16:25:19.7604424Z     #4 0x55e2c7 in LLVMFuzzerTestOneInput /src/selinux/checkpolicy/fuzz/checkpolicy-fuzzer.c:122:6
2021-09-30T16:25:19.7605595Z     #5 0x456b73 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) cxa_noexception.cpp
2021-09-30T16:25:19.7607020Z     #6 0x45636a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) cxa_noexception.cpp
2021-09-30T16:25:19.7608077Z     #7 0x457bdb in fuzzer::Fuzzer::MutateAndTestOne() cxa_noexception.cpp
2021-09-30T16:25:19.7609160Z     #8 0x458695 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) cxa_noexception.cpp
2021-09-30T16:25:19.7610309Z     #9 0x447e30 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) cxa_noexception.cpp
2021-09-30T16:25:19.7611517Z     #10 0x470ec2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
2021-09-30T16:25:19.7612606Z     #11 0x7fbdab3c10b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
2021-09-30T16:25:19.7613120Z 
2021-09-30T16:25:19.7613809Z DEDUP_TOKEN: __interceptor_malloc--define_cexpr--yyparse
2021-09-30T16:25:19.7614561Z Direct leak of 34 byte(s) in 4 object(s) allocated from:
2021-09-30T16:25:19.7615604Z     #0 0x52515d in __interceptor_malloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:129:3
2021-09-30T16:25:19.7616496Z     #1 0x56d648 in insert_id /src/selinux/checkpolicy/policy_define.c:126:18
2021-09-30T16:25:19.7617280Z     #2 0x5886a1 in yyparse /src/selinux/checkpolicy/policy_parse.y:877:31
2021-09-30T16:25:19.7618366Z     #3 0x55e2c7 in read_source_policy /src/selinux/checkpolicy/fuzz/checkpolicy-fuzzer.c:81:6
2021-09-30T16:25:19.7619677Z     #4 0x55e2c7 in LLVMFuzzerTestOneInput /src/selinux/checkpolicy/fuzz/checkpolicy-fuzzer.c:122:6
2021-09-30T16:25:19.7620847Z     #5 0x456b73 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) cxa_noexception.cpp
2021-09-30T16:25:19.7622062Z     #6 0x45636a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) cxa_noexception.cpp
2021-09-30T16:25:19.7623087Z     #7 0x457bdb in fuzzer::Fuzzer::MutateAndTestOne() cxa_noexception.cpp
2021-09-30T16:25:19.7624174Z     #8 0x458695 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) cxa_noexception.cpp
2021-09-30T16:25:19.7625322Z     #9 0x447e30 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) cxa_noexception.cpp
2021-09-30T16:25:19.7626493Z     #10 0x470ec2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
2021-09-30T16:25:19.7627575Z     #11 0x7fbdab3c10b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
2021-09-30T16:25:19.7628094Z 
2021-09-30T16:25:19.7628762Z DEDUP_TOKEN: __interceptor_malloc--insert_id--yyparse
2021-09-30T16:25:19.7629490Z Indirect leak of 40 byte(s) in 1 object(s) allocated from:
2021-09-30T16:25:19.7630722Z     #0 0x52515d in __interceptor_malloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:129:3
2021-09-30T16:25:19.7631673Z     #1 0x59177e in constraint_expr_init /src/selinux/libsepol/src/constraint.c:32:26
2021-09-30T16:25:19.7632510Z     #2 0x57d52b in define_cexpr /src/selinux/checkpolicy/policy_define.c:3752:6
2021-09-30T16:25:19.7633311Z     #3 0x587f2c in yyparse /src/selinux/checkpolicy/policy_parse.y:573:44
2021-09-30T16:25:19.7634420Z     #4 0x55e2c7 in read_source_policy /src/selinux/checkpolicy/fuzz/checkpolicy-fuzzer.c:81:6
2021-09-30T16:25:19.7635742Z     #5 0x55e2c7 in LLVMFuzzerTestOneInput /src/selinux/checkpolicy/fuzz/checkpolicy-fuzzer.c:122:6
2021-09-30T16:25:19.7636870Z     #6 0x456b73 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) cxa_noexception.cpp
2021-09-30T16:25:19.7652310Z     #7 0x45636a in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) cxa_noexception.cpp
2021-09-30T16:25:19.7653305Z     #8 0x457bdb in fuzzer::Fuzzer::MutateAndTestOne() cxa_noexception.cpp
2021-09-30T16:25:19.7654316Z     #9 0x458695 in fuzzer::Fuzzer::Loop(std::__Fuzzer::vector<fuzzer::SizedFile, std::__Fuzzer::allocator<fuzzer::SizedFile> >&) cxa_noexception.cpp
2021-09-30T16:25:19.7655682Z     #10 0x447e30 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) cxa_noexception.cpp
2021-09-30T16:25:19.7657015Z     #11 0x470ec2 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
2021-09-30T16:25:19.7658012Z     #12 0x7fbdab3c10b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
2021-09-30T16:25:19.7658456Z 
2021-09-30T16:25:19.7659178Z DEDUP_TOKEN: __interceptor_malloc--constraint_expr_init--define_cexpr
2021-09-30T16:25:19.7659981Z SUMMARY: AddressSanitizer: 122 byte(s) leaked in 6 allocation(s).
2021-09-30T16:25:19.7661659Z MS: 5 CrossOver-CMP-ChangeASCIIInt-ChangeByte-ChangeByte- DE: "netd"-; base unit: 1a5f664451a502a9bfd88b7bb74d570e3eabf548

It says

2021-09-30T16:25:20.0134894Z 2021-09-30 16:25:20,012 - root - INFO - Reproduce command returned: 0. Not reproducible on /github/workspace/build-out/checkpolicy-fuzzer.
2021-09-30T16:25:20.0144377Z 2021-09-30 16:25:20,013 - root - INFO - Crash is not reproducible.

Copy link
Contributor

@evverx evverx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at the build script and apart from min_pol.conf not being put into the seed corpus it looks good to me on the whole.

scripts/oss-fuzz.sh Outdated Show resolved Hide resolved
scripts/oss-fuzz.sh Outdated Show resolved Hide resolved
@@ -307,6 +307,7 @@ GLBLUB { return(GLBLUB); }
%%
int yyerror(const char *msg)
{
#ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a nice touch! I should have done something like that in the cil parser to prevent secilc-fuzzer from excessive logging.

@cgzones
Copy link
Contributor Author

cgzones commented Oct 1, 2021

I took a look at the build script and apart from min_pol.conf not being put into the seed corpus it looks good to me on the whole.

The fuzzer has the hardcoded setting to always expect a MLS policy (similar to checkpolicy -M), as the type of policy needs to be known a priory with the current parser implementation. I initially wrote also the non-MLS policy for testing, but it is intentionally not used currently (but I included it for reference and potential future use).

@cgzones
Copy link
Contributor Author

cgzones commented Oct 1, 2021

I'm not sure why CIFuzz didn't report a memory leak it found though:

Does oss-fuzz use some custom allocator, that fails unreproducible (to test error branches), cause there are probably a couple dozens memory leaks in error branches in the parser?

@evverx
Copy link
Contributor

evverx commented Oct 1, 2021

Does oss-fuzz use some custom allocator, that fails unreproducible (to test error branches), cause there are probably a couple dozens memory leaks in error branches in the parser?

No, it doesn't. I think that particular memory leak isn't reproducible because it can't be triggered by a single file (according to CIFuzz at least), which probably means that some kind of state accumulates between runs.

@evverx
Copy link
Contributor

evverx commented Oct 1, 2021

@cgzones could you temporarily apply the following patch:

diff --git a/.github/workflows/cifuzz.yml b/.github/workflows/cifuzz.yml
index 5c2233a2..b28eb71a 100644
--- a/.github/workflows/cifuzz.yml
+++ b/.github/workflows/cifuzz.yml
@@ -30,6 +30,7 @@ jobs:
           oss-fuzz-project-name: 'selinux'
           fuzz-seconds: 180
           dry-run: false
+          report-unreproducible-crashes: true
           sanitizer: ${{ matrix.sanitizer }}
       - name: Upload Crash
         uses: actions/upload-artifact@v1

so that CIFuzz could upload the file triggering that memory leak to hopefully make it easier to reproduce it locally.

@evverx
Copy link
Contributor

evverx commented Oct 1, 2021

could you temporarily apply the following patch

Done in #314

@cgzones cgzones force-pushed the checkpolicy_fuzz_patch branch 2 times, most recently from 1fafe97 to 120de3c Compare October 1, 2021 13:02
@cgzones
Copy link
Contributor Author

cgzones commented Oct 1, 2021

..., which probably means that some kind of state accumulates between runs.

True, the crash was an empty input, and after I added cleanup to some global variables it seems to be fixed.

@evverx
Copy link
Contributor

evverx commented Oct 1, 2021

FWIW I'm not sure why unreproducible crashes aren't reported by default by CIFuzz but I think I'd keep it on. OSS-Fuzz would just keep opening and closing "flaky" issues making it hard to to figure out what that was.

@evverx
Copy link
Contributor

evverx commented Oct 7, 2021

CIFuzz failed due to a bug in libClusterFuzz. It should be fixed in google/clusterfuzz#2471

Introduce a libfuzz[1] based fuzzer testing the parsing and policy
generation code used within checkpolicy(8) and checkmodule(8), similar
to the fuzzer for secilc(8).
The fuzzer will work on generated source policy input and try to parse,
link, expand, optimize, sort and output it.
This fuzzer will also ensure policy validation is not too strict by
checking compilable source policies are valid.

Build the fuzzer in the oss-fuzz script.

[1]: https://llvm.org/docs/LibFuzzer.html

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Close the input file and free all memory by the queue and lexer on a
syntax or parse error.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Free identifiers removed from the queue but not yet owned by the policy
on errors.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
…tion

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
The passed expression needs to be transferred into the policy or free'd
by the sink functions define_constraint() and define_validatetrans().

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Calling the parser macro YYABORT allows the parser to cleanup up any
allocated resources before returning.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Return early on invalid roles in user definition.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Convert the only usage of the raw type struct level_datum to use the
typedef.  Simplifies refactorizations on the type.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Add a new member to the struct level_datum to indicate whether the
member `level` is owned by the current instance, and free it on cleanup
only then.

This helps to implement a fix for a use-after-free issue in the
checkpolicy(8) compiler.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
During compilation sensitivity aliases share the level with their prime
sensitivity, until after the level has been fully defined they are
deduplicated.  If an error happens by that time the cleanup will free
the shared level multiple times, leading to a use-after-free.

Make use of the added new member of the struct level_datum.

Example policy:

    class c sid e class c{i}sensitivity S alias L;

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Provide more descriptive error messages by including the identifier
or other kind of value if available.

Also drop duplicate newlines at the end of messages.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Free the temporary bounds type in the error branches.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Only assign the computed value on success, since it is not set by
declare_symbol() on failure.

Reported by GCC:

    module_compiler.c: In function 'create_role':
    module_compiler.c:287:24: warning: use of uninitialized value 'value' [CWE-457] [-Wanalyzer-use-of-uninitialized-value]
      287 |         datum->s.value = value;
          |         ~~~~~~~~~~~~~~~^~~~~~~

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
Sync function parameter names.

Drop superfluous return value.

  The function avrule_merge_ioctls() has no failure conditions and
  always returns 0.

Drop duplicate include.

Use native type for ranges.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
@cgzones cgzones changed the base branch from master to main January 22, 2024 13:38
@cgzones cgzones marked this pull request as ready for review January 22, 2024 13:38
@cgzones cgzones closed this Mar 11, 2024
@cgzones cgzones deleted the checkpolicy_fuzz_patch branch March 11, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants